[GS] Show all applications when opening the search bar #78741

pgayvallet · 2020-09-29T09:11:48Z

Summary

change the defaultMaxProviderResults constant to allow all apps to be displayed
add circuit breaker on SO provider to avoid performing a search when the term is empty
add search subscription management to the search bar component to avoid displaying previous results in case of network issues

Checklist

Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2020-09-29T09:19:36Z

Note: The 'display apps only when the term is empty' logic that is in the searchbar bothers me a little. Ideally, in that case, we would just not be performing a GS search and, instead, retrieving the full app list from core's application.applications$ API. That would avoid unecessary calls to the backend, and avoid having to increase this max result constant to a value that is too high for other providers. Also, the GS API should just not perform any call to the providers when the term is empty.

I didn't do that change in the current PR as I wasn't sure of the future of the 'default state' of the searchbar menu. @ryankeairns is that alright if I open an issue to do this optimization, or are we planning to display other things than just apps when the search term is empty in the near futur?

@myasonik: I think I found a UI bug regarding the searchbar:

Open the searchbar by clicking on it
Type apm -> only the apm app result is displayed
Click on the apm result -> you are redirected to the apm app, and the searchText is cleared
click on the searchbar again -> the search term is empty, but the previous results (only apm) are displayed.

I'm guessing this is an unwanted behavior and that we would like to have all results displayed again here?

…l-apps

ryankeairns · 2020-09-29T13:19:47Z

Note: The 'display apps only when the term is empty' logic that is in the searchbar bothers me a little. Ideally, in that case, we would just not be performing a GS search and, instead, retrieving the full app list from core's application.applications$ API. That would avoid unecessary calls to the backend, and avoid having to increase this max result constant to a value that is too high for other providers. Also, the GS API should just not perform any call to the providers when the term is empty.

I didn't do that change in the current PR as I wasn't sure of the future of the 'default state' of the searchbar menu. @ryankeairns is that alright if I open an issue to do this optimization, or are we planning to display other things than just apps when the search term is empty in the near future?

@pgayvallet we have this issue to track redesigning the default state. Several ideas have been circulated, none of which individually stood above the rest, so our decision was to release with the app results. From there, we will gather feedback from customers and evaluate whether or not we should move to something else.

The technical implications of the current situation hadn't occurred to me and make a good deal of sense. What I don't have a grasp on is how much of an issue this presents. I would defer to the Platform team on whether or not this should be fixed now versus waiting for another release cycle.

Thanks for bringing this up and explaining it thoroughly.

pgayvallet · 2020-09-29T13:48:26Z

The technical implications of the current situation hadn't occurred to me and make a good deal of sense. What I don't have a grasp on is how much of an issue this presents

The critically is low until we have additional providers. This can definitely wait for the next release cycle.

ryankeairns · 2020-09-29T13:58:52Z

Ok, understood. I've created a separate issue to track this and linked it to the 'default state redesign' issue. Thanks!

pgayvallet · 2020-09-29T14:43:59Z

@myasonik FYI, created #78786 and #78785

elasticmachine · 2020-09-29T17:03:12Z

Pinging @elastic/kibana-core-ui (Team:Core UI)

elasticmachine · 2020-09-29T17:03:13Z

Pinging @elastic/kibana-platform (Team:Platform)

ryankeairns · 2020-10-01T21:38:37Z

Quick reminder that this one is critical for 7.10. Let me know if I can be of any assistance getting it all cleared. Thanks!

pgayvallet · 2020-10-02T05:45:12Z

x-pack/plugins/global_search_providers/server/providers/saved_objects/provider.ts

+      if (!term) {
+        return of([]);
+      }


Just a quick circuit breaker to avoid unnecessary ES calls until we discuss more in details of the expected behavior for empty term searches in #78771

pgayvallet · 2020-10-02T05:46:33Z

x-pack/plugins/global_search_bar/public/components/search_bar.tsx

+      // cancel pending search if not completed yet
+      if (searchSubscription.current) {
+        searchSubscription.current.unsubscribe();
+        searchSubscription.current = null;
+      }


Other changes in the file are just cleanup. I just added previous search unsubscription to avoid previous results to be displayed in case of network issue (previous call finishing after next call)

Should we have a test for this to avoid breaking in the future?

joshdover

If practical, I think a test on the search bar behavior would be good, but otherwise the changes LGTM

joshdover · 2020-10-02T18:20:48Z

x-pack/plugins/global_search_bar/public/components/search_bar.tsx

+      // cancel pending search if not completed yet
+      if (searchSubscription.current) {
+        searchSubscription.current.unsubscribe();
+        searchSubscription.current = null;
+      }


Should we have a test for this to avoid breaking in the future?

…l-apps

kibanamachine · 2020-10-05T09:03:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 70fb894

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`globalSearch`	22	23	+1

distributable file count

id	before	after	diff
`default`	47084	47085	+1

page load bundle size

id	before	after	diff
`globalSearchBar`	28.7KB	29.0KB	+321.0B

History

💚 Build #77915 succeeded b8810e0
💔 Build #77888 failed c3f55e8
💔 Build #77797 failed 559853e
💔 Build #77777 failed 3aca649

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* increase default number of results to show all apps * fix circuit breaker * fix ut * add unit test

increase default number of results to show all apps

3aca649

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 labels Sep 29, 2020

Merge remote-tracking branch 'upstream/master' into kbn-78174-show-al…

559853e

…l-apps

ryankeairns mentioned this pull request Sep 29, 2020

[GS] Improve the display logic for the default state of navigational search #78771

Open

fix circuit breaker

c3f55e8

fix ut

b8810e0

pgayvallet marked this pull request as ready for review September 29, 2020 17:03

pgayvallet requested a review from a team as a code owner September 29, 2020 17:03

pgayvallet requested a review from a team September 29, 2020 17:03

myasonik approved these changes Sep 29, 2020

View reviewed changes

pgayvallet commented Oct 2, 2020

View reviewed changes

joshdover approved these changes Oct 2, 2020

View reviewed changes

pgayvallet added 2 commits October 5, 2020 07:58

Merge remote-tracking branch 'upstream/master' into kbn-78174-show-al…

0689613

…l-apps

add unit test

70fb894

pgayvallet merged commit 1e9135c into elastic:master Oct 5, 2020

pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Oct 5, 2020

[GS] Show all applications when opening the search bar (elastic#78741)

f196905

* increase default number of results to show all apps * fix circuit breaker * fix ut * add unit test

pgayvallet mentioned this pull request Oct 5, 2020

[7.x] [GS] Show all applications when opening the search bar (#78741) #79422

Merged

pgayvallet added a commit that referenced this pull request Oct 5, 2020

[GS] Show all applications when opening the search bar (#78741) (#79422)

2ace108

* increase default number of results to show all apps * fix circuit breaker * fix ut * add unit test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GS] Show all applications when opening the search bar #78741

[GS] Show all applications when opening the search bar #78741

pgayvallet commented Sep 29, 2020 •

edited

Loading

pgayvallet commented Sep 29, 2020 •

edited

Loading

ryankeairns commented Sep 29, 2020

pgayvallet commented Sep 29, 2020

ryankeairns commented Sep 29, 2020 •

edited

Loading

pgayvallet commented Sep 29, 2020

elasticmachine commented Sep 29, 2020

elasticmachine commented Sep 29, 2020

ryankeairns commented Oct 1, 2020

pgayvallet Oct 2, 2020

pgayvallet Oct 2, 2020

joshdover Oct 2, 2020

joshdover left a comment

joshdover Oct 2, 2020

kibanamachine commented Oct 5, 2020

[GS] Show all applications when opening the search bar #78741

[GS] Show all applications when opening the search bar #78741

Conversation

pgayvallet commented Sep 29, 2020 • edited Loading

Summary

Checklist

pgayvallet commented Sep 29, 2020 • edited Loading

ryankeairns commented Sep 29, 2020

pgayvallet commented Sep 29, 2020

ryankeairns commented Sep 29, 2020 • edited Loading

pgayvallet commented Sep 29, 2020

elasticmachine commented Sep 29, 2020

elasticmachine commented Sep 29, 2020

ryankeairns commented Oct 1, 2020

pgayvallet Oct 2, 2020

Choose a reason for hiding this comment

pgayvallet Oct 2, 2020

Choose a reason for hiding this comment

joshdover Oct 2, 2020

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

joshdover Oct 2, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 5, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

distributable file count

page load bundle size

History

pgayvallet commented Sep 29, 2020 •

edited

Loading

pgayvallet commented Sep 29, 2020 •

edited

Loading

ryankeairns commented Sep 29, 2020 •

edited

Loading